Skip to content

fix(transactions): include enable_banking in pending/confirmed status filter#1885

Merged
jjmata merged 3 commits into
we-promise:mainfrom
web-dev0521:fix/transaction-status-filter-enable-banking
May 31, 2026
Merged

fix(transactions): include enable_banking in pending/confirmed status filter#1885
jjmata merged 3 commits into
we-promise:mainfrom
web-dev0521:fix/transaction-status-filter-enable-banking

Conversation

@web-dev0521
Copy link
Copy Markdown
Contributor

@web-dev0521 web-dev0521 commented May 20, 2026 •
edited by coderabbitai Bot
Loading

Summary

The transaction status filter (Pending / Confirmed) hardcoded only simplefin, plaid, and lunchflow in its SQL, even though Transaction::PENDING_PROVIDERS also includes enable_banking. As a result,
for Enable Banking users:

  • "Pending" filter → 0 results — Enable Banking pending transactions never matched the filter SQL.
  • "Confirmed" filter leaked pending txns — the confirmed condition didn't check enable_banking, so those transactions passed through.

This is the same class of bug that was previously fixed for lunchflow in #859.

Fixes #1668.

Changes

  • Transaction::Search#apply_status_filter now delegates to the existing pending / excluding_pending model scopes instead of hardcoded SQL.
  • EntrySearch#apply_status_filter now interpolates Transaction::PENDING_CHECK_SQL (already aliased to t) into its EXISTS subqueries.
  • Both paths are now sourced from Transaction::PENDING_PROVIDERS, so adding a future provider can't silently reintroduce the bug.
  • Added a regression test that creates one pending transaction per provider in PENDING_PROVIDERS and asserts each is matched by "Pending" and excluded from "Confirmed".

Test plan

  • bin/rails test test/models/transaction/search_test.rb
  • Manually: with an Enable Banking account that has pending transactions, filter Transactions by Status: Pending → pending txns appear; filter by Status: Confirmed → pending txns are excluded.

Summary by CodeRabbit

  • Bug Fixes

    • Improved transaction/entry status filtering so pending and confirmed states are applied reliably and consistently across supported providers.

  • Tests

    • Added regression tests verifying status filters correctly include pending items and exclude them from confirmed results for all supported providers.

Review Change Stack

@superagent-security superagent-security Bot added the contributor:verified Contributor passed trust analysis. label May 20, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 20, 2026 •
edited
Loading

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5ed966ad-e230-4d48-8672-94a8d7e56bdf

📥 Commits

Reviewing files that changed from the base of the PR and between bd7170d and e8c6472.

📒 Files selected for processing (1)
  • test/models/account/entry_test.rb
📝 Walkthrough

Walkthrough

Centralizes pending detection into Transaction::PENDING_CHECK_SQL and delegates status filtering to model scopes; updates EntrySearch to use the shared predicate and adds regression tests that verify pending/confirmed filtering across all providers.

Changes

Status Filter Refactoring

Layer / File(s) Summary
Refactor Transaction::Search to delegate to model scopes
app/models/transaction/search.rb		 apply_status_filter replaces inline provider-specific SQL with a case that applies query.pending for ["pending"] and query.excluding_pending for ["confirmed"].
Consolidate pending check logic in EntrySearch
app/models/entry_search.rb		 pending_condition and the NOT EXISTS confirmed_condition now interpolate the shared Transaction::PENDING_CHECK_SQL (aliasing transactions as t) instead of hardcoding provider JSON path checks.
Add regression tests for status filter across providers
test/models/transaction/search_test.rb, test/models/account/entry_test.rb		 New tests create one pending transaction/entry per provider in Transaction::PENDING_PROVIDERS and assert that status: ["pending"] includes those and status: ["confirmed"] excludes them (and vice versa for a confirmed record).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • we-promise/sure#859: Both PRs modify SQL predicates in status filtering; this PR consolidates provider checks into shared logic, superseding provider-specific checks introduced earlier.
  • we-promise/sure#1783: Related to handling of provider-specific pending flags and pending→posted auto-claim behavior touching the same transaction.extra flags.

Suggested labels

contributor:verified, pr:verified

Suggested reviewers

  • jjmata

Poem

🐰 In code where statuses once split apart,
I stitched the checks to share a single heart.
Pending flags now follow one neat line,
Tests hop through providers — each one fine.
With carrots and bytes, our filters shine.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main fix: including enable_banking in the pending/confirmed status filter.
Linked Issues check ✅ Passed The code changes address all objectives from #1668: the status filter now works correctly for all providers including enable_banking by delegating to model scopes and interpolating shared SQL predicates.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the status filter for pending/confirmed transactions across all providers as specified in #1668.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@superagent-security superagent-security Bot added the pr:verified PR passed security analysis. label May 20, 2026
@web-dev0521
fix(transactions): include enable_banking in pending/confirmed status…
bd7170d 
… filter (we-promise#1668)

The transaction status filter hardcoded only simplefin/plaid/lunchflow in
its pending/confirmed SQL, even though Transaction::PENDING_PROVIDERS also
includes enable_banking. As a result, Enable Banking pending transactions
returned 0 results under the "Pending" filter and leaked into "Confirmed".

Source the provider list from the existing constant-driven helpers instead:
- Transaction::Search delegates to the `pending` / `excluding_pending` model
  scopes.
- EntrySearch interpolates Transaction::PENDING_CHECK_SQL into its EXISTS
  subqueries.

This keeps every status-filter path in sync with PENDING_PROVIDERS so adding
a future provider can't reintroduce the bug.

Fixes we-promise#1668
@web-dev0521 web-dev0521 force-pushed the fix/transaction-status-filter-enable-banking branch from 1e0c28d to bd7170d Compare May 20, 2026 21:15
@jjmata Claude
Copy link
Copy Markdown
Collaborator

jjmata commented May 21, 2026

Good fix and the right architectural direction — delegating to pending/excluding_pending scopes means any future provider added to PENDING_PROVIDERS is automatically covered in both filter paths without touching search code.

One thing to verify: EntrySearch#apply_status_filter now embeds Transaction::PENDING_CHECK_SQL directly in the EXISTS subquery that already aliases transactions t. Double-check that PENDING_CHECK_SQL is written to reference the table as t (or is alias-agnostic) so the interpolation doesn't produce a broken query. If the constant uses transactions.extra unqualified, it would conflict with the t alias in the subquery context. Worth adding a quick integration test for the EntrySearch path (the existing regression test covers Transaction::Search — are both paths exercised?).

Generated by Claude Code

@web-dev0521
test(entry): cover EntrySearch status filter across all pending provi…
e8c6472 
…ders

Adds a regression test for the EntrySearch#apply_status_filter path,
asserting pending transactions for every PENDING_PROVIDERS entry are
matched by the "pending" filter and excluded from "confirmed". Mirrors
the existing Transaction::Search coverage so both filter paths are
exercised.
@web-dev0521 web-dev0521 force-pushed the fix/transaction-status-filter-enable-banking branch from 0935ff3 to e8c6472 Compare May 21, 2026 01:37
@sure-design sure-design mentioned this pull request May 21, 2026
Open
3 tasks
@web-dev0521
Copy link
Copy Markdown
Contributor Author

web-dev0521 commented May 27, 2026

Hi, @jjmata ,
Could you please review this PR?

@superagent-security superagent-security Bot removed the contributor:verified Contributor passed trust analysis. label May 31, 2026
@web-dev0521
Copy link
Copy Markdown
Contributor Author

web-dev0521 commented May 31, 2026

Hi, @jjmata ,
Could you please review this PR?

@superagent-security superagent-security Bot removed the pr:verified PR passed security analysis. label May 31, 2026
@jjmata jjmata self-requested a review May 31, 2026 20:09
@jjmata jjmata added this to the v0.7.2 milestone May 31, 2026
@jjmata jjmata merged commit 3311759 into we-promise:main May 31, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jjmata jjmata jjmata approved these changes

Assignees

@web-dev0521 web-dev0521

Labels

None yet

Projects

Current `latest` release train (v0.7.2) - June 2026
Status: Done

Milestone

v0.7.2

Development

Successfully merging this pull request may close these issues.

Bug: transaction status filter not working correctly

2 participants

@web-dev0521 @jjmata